OAuth2: PKCE(Proof Key for Code Exchange) by zhaohuabing · Pull Request #37849 · envoyproxy/envoy

zhaohuabing · 2025-01-02T06:47:47Z

This PR introduces support for PKCE(Proof Key for Code Exchange) in the OAuth2 filter. This enhancement mitigates the risk of the authorization code interception attacks.

Background: https://oauth.net/2/pkce/
RFC: Proof Key for Code Exchange by OAuth Public Clients

Commit Message:
Additional Description:
Risk Level: low
Testing: unit and integrate test, also manually tested with AWS cognito
Docs Changes:
Release Notes: Yes
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #35230]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

CC @missBerg @arkodg @denniskniep

repokitteh-read-only · 2025-01-02T06:47:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37849 was opened by zhaohuabing.

see: more, trace.

api/envoy/extensions/filters/http/oauth2/v3/oauth.proto

denniskniep · 2025-01-08T10:18:56Z

Hi @zhaohuabing,
can´t we add a new parameter code_verifier to the asyncGetAccessToken method.
This is then used here:

envoy/source/extensions/filters/http/oauth2/filter.cc

Lines 467 to 468 in 2425431

    
           oauth_client_->asyncGetAccessToken(auth_code_, config_->clientId(), config_->clientSecret(), 
        
                                              redirect_uri, config_->authType());

The parameter is set by validateOAuthCallback return value (CallbackValidationResult), which retrieves it from a newly introduced code_verifier cookie (http-only, secure).

envoy/source/extensions/filters/http/oauth2/filter.cc

Line 455 in 2425431

    
           const CallbackValidationResult result = validateOAuthCallback(headers, path_str);

This newly introduced code_verifier cookie is set in the redirectToOAuthServer method.

zhaohuabing · 2025-01-09T02:18:50Z

@denniskniep Yes, you'r right about this. The asyncGetAcessToken is triggered by the IDP's redirect after user authentication, and we can retrieve the code verifier from the cookie. I missed that - thanks for pointing it out!

phlax · 2025-01-11T14:15:17Z

... coverage check failed ...

should be fixed now

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

source/extensions/filters/http/oauth2/filter.cc

Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

zhaohuabing · 2025-03-19T13:57:03Z

@denniskniep @wbpcode Whenever you have a moment, could you take another look at this PR? I'd love to wrap this up soon and move on to other OAuth2 improvement tasks. Really appreciate your time - thanks!

zhaohuabing · 2025-03-20T01:43:21Z

/retest

zhaohuabing · 2025-03-22T01:15:02Z

ping

wbpcode

LGTM. Thanks for this great contribution and sorry for the delay.

wbpcode · 2025-03-22T13:49:21Z

I think now you can ping the @phlax to merge the example PR?

zhaohuabing · 2025-03-25T02:30:20Z

@wbpcode Do we need to wait until the example is updated in the bazel configuration?

wbpcode

LGTM. Thanks.

wbpcode · 2025-03-25T13:22:31Z

@wbpcode Do we need to wait until the example is updated in the bazel configuration?

Yeah, I think so.

phlax · 2025-03-25T13:30:31Z

i was hoping to do some docs cleanups before landing #38872 but i can do that in a follow up if its blocking

zhaohuabing · 2025-03-25T13:40:14Z

i was hoping to do some docs cleanups before landing #38872 but i can do that in a follow up if its blocking

@phlax Yeah, I would appreciate it if you could land #38872 sooner as it blocks this one.

phlax · 2025-03-25T13:42:18Z

done

This reverts commit 7eeffc1. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

wbpcode

LGTM. Thanks.

Introduced by #37849 It is clear that `code_verifier_cookie_settings_` is not set to the `FilterConfig`, and only the default is used. Commit Message: Additional Description: Risk Level: low Testing: unit test Docs Changes: Signed-off-by: Boteng Yao <boteng@google.com>

@missBerg

This PR introduces support for PKCE(Proof Key for Code Exchange) in the OAuth2 filter. This enhancement mitigates the risk of the authorization code interception attacks. Background: https://oauth.net/2/pkce/ RFC: [Proof Key for Code Exchange by OAuth Public Clients](https://datatracker.ietf.org/doc/html/rfc7636) Commit Message: Additional Description: Risk Level: low Testing: unit and integrate test, also manually tested with AWS cognito Docs Changes: Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes envoyproxy#35230] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] CC @missBerg @arkodg @denniskniep --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

) Introduced by envoyproxy#37849 It is clear that `code_verifier_cookie_settings_` is not set to the `FilterConfig`, and only the default is used. Commit Message: Additional Description: Risk Level: low Testing: unit test Docs Changes: Signed-off-by: Boteng Yao <boteng@google.com>

@missBerg

This PR introduces support for PKCE(Proof Key for Code Exchange) in the OAuth2 filter. This enhancement mitigates the risk of the authorization code interception attacks. Background: https://oauth.net/2/pkce/ RFC: [Proof Key for Code Exchange by OAuth Public Clients](https://datatracker.ietf.org/doc/html/rfc7636) Commit Message: Additional Description: Risk Level: low Testing: unit and integrate test, also manually tested with AWS cognito Docs Changes: Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes envoyproxy#35230] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] CC @missBerg @arkodg @denniskniep --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

) Introduced by envoyproxy#37849 It is clear that `code_verifier_cookie_settings_` is not set to the `FilterConfig`, and only the default is used. Commit Message: Additional Description: Risk Level: low Testing: unit test Docs Changes: Signed-off-by: Boteng Yao <boteng@google.com>

repokitteh-read-only bot added the api label Jan 2, 2025

repokitteh-read-only bot assigned wbpcode Jan 2, 2025

zhaohuabing commented Jan 2, 2025

View reviewed changes

api/envoy/extensions/filters/http/oauth2/v3/oauth.proto Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

zhaohuabing closed this Jan 2, 2025

zhaohuabing reopened this Jan 9, 2025

zhaohuabing force-pushed the api-ouath-pkce branch from 9d27608 to 6ad7158 Compare January 9, 2025 04:19

zhaohuabing marked this pull request as draft January 9, 2025 04:19

zhaohuabing marked this pull request as ready for review January 9, 2025 09:07

zhaohuabing changed the title ~~OAuth2: API for PKCE(Proof Key for Code Exchange)~~ OAuth2: PKCE(Proof Key for Code Exchange) Jan 9, 2025

zhaohuabing force-pushed the api-ouath-pkce branch from 8cc5214 to 72795d6 Compare January 10, 2025 07:37

This comment was marked as outdated.

Sign in to view

zhaohuabing force-pushed the api-ouath-pkce branch from a06c880 to 5ade7e5 Compare January 12, 2025 04:04

zhaohuabing added 8 commits January 12, 2025 04:09

api for pkce(Proof Key for Code Exchange)

21313ef

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

implementation

7dbbde5

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

update tests

c008e80

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

minor wording

aac2652

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

add change log

0dd6e3a

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix format

60e5bab

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix test

6df85ea

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

remove enable_pkce

62f5537

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing force-pushed the api-ouath-pkce branch from 5ade7e5 to 62f5537 Compare January 12, 2025 04:09

wbpcode reviewed Jan 14, 2025

View reviewed changes

source/extensions/filters/http/oauth2/filter.cc Show resolved Hide resolved

zhaohuabing requested a review from wbpcode January 15, 2025 01:50

zhaohuabing added 2 commits January 15, 2025 10:02

Merge branch 'main' into api-ouath-pkce

73860ef

Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

encrypt the code verifier

3cf2b43

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing requested a review from denniskniep March 14, 2025 08:52

Merge branch 'main' into api-ouath-pkce

ac3b502

Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

repokitteh-read-only bot removed the waiting label Mar 19, 2025

wbpcode approved these changes Mar 22, 2025

View reviewed changes

repokitteh-read-only bot removed the api label Mar 22, 2025

wbpcode previously approved these changes Mar 25, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into api-ouath-pkce

05a54cd

zhaohuabing dismissed wbpcode’s stale review via 53beb52 March 25, 2025 14:00

zhaohuabing requested a review from wbpcode March 25, 2025 14:03

Revert "fix verify"

69211f5

This reverts commit 7eeffc1. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>

zhaohuabing force-pushed the api-ouath-pkce branch from 53beb52 to 69211f5 Compare March 25, 2025 14:04

wbpcode approved these changes Mar 26, 2025

View reviewed changes

wbpcode merged commit 0679277 into envoyproxy:main Mar 26, 2025
25 checks passed

zhaohuabing deleted the api-ouath-pkce branch March 26, 2025 04:08

botengyao mentioned this pull request Mar 29, 2025

oauth2: fix the code_verifier_cookie config is not set #38963

Merged

zhaohuabing mentioned this pull request Apr 2, 2025

api(oidc): Allow override of all OIDC CookieNames envoyproxy/gateway#5655

Closed

Conversation

zhaohuabing commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 2, 2025

Uh oh!

Uh oh!

This comment was marked as resolved.

denniskniep commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaohuabing commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

phlax commented Jan 11, 2025

Uh oh!

Uh oh!

zhaohuabing commented Mar 19, 2025

Uh oh!

zhaohuabing commented Mar 20, 2025

Uh oh!

zhaohuabing commented Mar 22, 2025

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 22, 2025

Uh oh!

zhaohuabing commented Mar 25, 2025

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 25, 2025

Uh oh!

phlax commented Mar 25, 2025

Uh oh!

zhaohuabing commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phlax commented Mar 25, 2025

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhaohuabing commented Jan 2, 2025 •

edited

Loading

denniskniep commented Jan 8, 2025 •

edited

Loading

zhaohuabing commented Jan 9, 2025 •

edited

Loading

zhaohuabing commented Mar 25, 2025 •

edited

Loading